Updated LRO guidelines #517

JeffreyRichter · 2024-01-17T18:52:59Z

No description provided.

mikekistler

I've left some suggestions to fix minor issues, but I have a larger concern. It looks like this PR has dropped many of the prior guidelines we had in place for LROs. A quick check on the href tags shows all the following guidelines were dropped.

< #lro-no-post-create
< #lro-operation-id-default-is-guid
< #lro-operation-id-request-header
< #lro-operation-id-unique-except-retries
< #lro-put-operation-id-default-is-guid
< #lro-put-operation-id-request-header
< #lro-put-operation-id-unique-except-retries
< #lro-put-operation-location-includes-api-version
< #lro-put-returns-200-or-201
< #lro-put-returns-operation-id-header
< #lro-put-returns-operation-location
< #lro-put-valid-inputs-synchronously
< #lro-returns-202
< #lro-returns-only-202
< #lro-returns-status-monitor
< #lro-status-monitor-accepts-any-api-version
< #lro-status-monitor-get-returns-200
< #lro-status-monitor-includes-all-fields
< #lro-status-monitor-no-resource-result
< #lro-status-monitor-post-action-result
< #lro-status-monitor-retry-after
< #lro-status-monitor-structure

This has me concerned that the PR is a "more revolutionary than evolutionary" change to our LRO guidelines, which is not what I had understood from your word doc.

Maybe the guidelines above are retained but just their tags were lost?

Or is this a more wholesale change?

azure/Guidelines.md

mikekistler · 2024-01-19T16:02:17Z

azure/Guidelines.md

-The second pattern applies only in the case of a PUT operation to create a resource that also involves additional long-running processing.
-For guidance on when to use a specific pattern, please refer to [Considerations for Service Design, Long Running Operations](./ConsiderationsForServiceDesign.md#long-running-operations).
-These are described in the following two sections.
+## Patterns to Initiate a Long-Running Operation


Suggested change

## Patterns to Initiate a Long-Running Operation

#### Patterns to Initiate a Long-Running Operation

mikekistler · 2024-01-19T16:03:06Z

azure/Guidelines.md


-**OperationStatus** : Object
+## The Status Monitor Resource


Suggested change

## The Status Monitor Resource

#### The Status Monitor Resource

mikekistler · 2024-01-19T16:03:31Z

azure/Guidelines.md


-<a href="#lro-status-monitor-includes-all-fields" name="lro-status-monitor-includes-all-fields">:white_check_mark:</a> **DO** include the `id` of the operation and any other values needed for the client to form a GET request to the status monitor (e.g. a `location` path parameter).
+## Pattern to Poll a Long-Running Operation's Status


Suggested change

## Pattern to Poll a Long-Running Operation's Status

#### Pattern to Poll a Long-Running Operation's Status

mikekistler · 2024-01-19T16:04:00Z

azure/Guidelines.md


 <a href="#lro-status-monitor-retention" name="lro-status-monitor-retention">:white_check_mark:</a> **DO** retain the status monitor resource for some publicly documented period of time (at least 24 hours) after the operation completes.

+## Pattern to List Status Monitors (optional)


Suggested change

## Pattern to List Status Monitors (optional)

#### Pattern to List Status Monitors (optional)

JeffreyRichter · 2024-01-19T17:32:51Z

@mikekistler, I want to address this comment: #517 (review)

This is a wholesale rewrite of the guideline text but all the old guidelines remain. I struggled without how to get the guidelines across in a simplified manner. I untimately settled on what worked during our discussions with my Word document. That is, the HTTP requests/responses themselves in the new guidelines SHOW exactly what to do as opposed to explaining via text what to do. I think this makes it easier to follow and is more prescriptive for service teams.

If you pick some text guideline that was there before and read the new guidelines, I'm very sure you'll find the issue still covered but by way of prescriptive example.

heaths

I see a lot of formatting inconsistencies that will make a difference, but I'm mostly concerned about switching to a single directive (that we can right-click and copy the link to send in reviews, PR reviews, etc.) with a lot of bullet points beneath. That's not how we've written these guidelines previously. Even though it might make sense to group them that way, I think the bullets should still have the DOs and SHOULDs and all that, along with bookmarks.

Where possible, we should try to retain existing bookmarks - even if we create new ones (just use the <a name> in that case) because we have been linking these out in reviews.

By no means am I suggesting we shouldn't drop things when it makes sense - I'm 100% behind updating guidelines as we learn better patterns or need to clarify - but let's try to keep this stable like we would any REST API. These guidelines are also living contract like swaggers.

azure/Guidelines.md

heaths · 2024-01-19T21:45:54Z

azure/Guidelines.md

-through another API call.
-See the [Long Running Operations section](./ConsiderationsForServiceDesign.md#long-running-operations) in
-Considerations for Service Design for an introduction to the design of long-running operations.
+A _long-running operation (LRO)_ is typically an operation that should execute synchronously but due to services not wanting to maintain long-lived connections (>1 seconds) and load-balancer timeouts the operation must execute asynchronously. For this pattern, the client initiates the operation on the service and then the client repeatedly polls the service (via another API call) to track the operation's progress/completion. 


is typically an operation that should execute synchronously

A lot of the LROs we've reviewed don't fall into this, and it seems the norm - especially for very LROs like cert signing, AI tagging, etc. - is quickly growing to truly LROs. I think this assertion is misleading and could make people doubt whether they should be using an LRO or not. The original text seemed fine.

azure/Guidelines.md

heaths · 2024-01-19T21:57:15Z

azure/Guidelines.md


-<a href="#lro-valid-inputs-synchronously" name="lro-valid-inputs-synchronously">:white_check_mark:</a> **DO** perform as much validation as practical when initiating the operation to alert clients of errors early.
+  - The resource schema must contain an `initializationStatus` property indicating whether the initialization is `Running`, `Failed`, `Succeeded`, etc.


I'm not crazy about "etc". We document the supported / recommended values. Either list them here, or link to them (I believe they are above).

I assume this is the same PUT to create a resource (as we support today).

On the initializationStatus property: so we got another "required" property that would only exist in future LRO, but not in existing LRO?

Trying to see if we can avoid the nuance of "legacy LRO", "LRO without initializationStatus", "LRO with initializationStatus and kind"...

heaths · 2024-01-19T22:04:12Z

azure/Guidelines.md

@@ -1098,8 +1180,65 @@ While it may be tempting to use a revision/version number for the resource as th

 <a href="#condreq-etag-depends-on-encoding" name="condreq-etag-depends-on-encoding">:white_check_mark:</a> **DO**, when supporting multiple representations (e.g. Content-Encodings) for the same resource, generate different ETag values for the different representations.

+<a href="#substrings" name="substrings"></a>


FWIW, for headers this is pointless. It's not what commonmark will use during rendering, which is what GitHub et. al. use.

heaths · 2024-01-19T22:05:17Z

azure/Guidelines.md

+
+For example, if a service response needed to identify offset & length values for "name" and "email" substrings, the JSON response would look like this:
+
+```


Suggested change

```

```json

Will properly syntax highlight it.

...though, I'd then elide line 1201 below since it won't be valid and their json highlighter doesn't like comments. You might see if jsonc works.

heaths · 2024-01-19T22:07:04Z

azure/Guidelines.md

+```
+{
+  (... other properties not shown...)
+  "fullString": "(...some string containing a name and an email address...)",


Should we actually include a working example with emoji or international characters so people perhaps get a better sense of when simple indexes or lengths fail?

heaths · 2024-01-19T22:08:24Z

azure/Guidelines.md

+```
+   var response := client.SomeMethodReturningJSONShownAbove(...)
+   name := response.fullString[ response.name.offset.utf8 : response.name.offset.utf8 + response.name.length.utf8]
+```


Might as well show proper syntax highlighting (and code):

Suggested change

```

var response := client.SomeMethodReturningJSONShownAbove(...)

name := response.fullString[ response.name.offset.utf8 : response.name.offset.utf8 + response.name.length.utf8]

```

```go

response := client.SomeMethodReturningJSONShownAbove("...")

name := response.fullString[ response.name.offset.utf8 : response.name.offset.utf8 + response.name.length.utf8]

```

heaths · 2024-01-19T22:10:36Z

azure/Guidelines.md

+   name := response.fullString[ response.name.offset.utf8 : response.name.offset.utf8 + response.name.length.utf8]
+```
+
+The service must calculate the offset & length for all 3 encodings and return them because clients find it difficult working with Unicode encodings and how to convert from one encoding to another. In other words, we do this to simplify client development and ensure customer success when isolating a substring.


I realize we've already discussed this, but I find this statement contradictory. It is a hard problem, but why is them passing the encoding any different than them knowing which encoding to use coming back in the response?

I still think this is something that our 1P clients - generated or otherwise - should handle for them. It could still be the same response - I know that helps solve problems of calling into one service that calls another service - but we can still make it easy by helping select - or just do it for them - the right encoding for the language SDK being used.

weidongxu-microsoft · 2024-01-25T09:51:42Z

azure/Guidelines.md


-<a href="#lro-valid-inputs-synchronously" name="lro-valid-inputs-synchronously">:white_check_mark:</a> **DO** perform as much validation as practical when initiating the operation to alert clients of errors early.
+  - The resource schema must contain an `initializationStatus` property indicating whether the initialization is `Running`, `Failed`, `Succeeded`, etc.


I assume this is the same PUT to create a resource (as we support today).

On the initializationStatus property: so we got another "required" property that would only exist in future LRO, but not in existing LRO?

Trying to see if we can avoid the nuance of "legacy LRO", "LRO without initializationStatus", "LRO with initializationStatus and kind"...

weidongxu-microsoft · 2024-01-25T09:56:00Z

azure/Guidelines.md


-For a PUT (create or replace) with additional long-running processing:
+- Do not return a response body.


Does the resource disappear right now (after the DELETE), or it still there and only disappear after LRO success? If latter, how's it initializationStatus (or other state) if queried via GET?

azure/Guidelines.md

azure/ConsiderationsForServiceDesign.md

azure/Guidelines.md

azure/ConsiderationsForServiceDesign.md

azure/Guidelines.md

mikekistler · 2024-04-09T02:04:39Z

@markcowl @allenjzhang For your awareness. Once these guidelines are finalized we should add support in TypeSpec for the new patterns.

weidongxu-microsoft · 2024-04-09T08:57:25Z

A comment on status property in the response of the PUT, for "PUT to resource with additional long-running processing".
#517 (comment)

I don't think we should add the status to resource properties. It would be status monitor property.

azure/ConsiderationsForServiceDesign.md

azure/Guidelines.md

Co-authored-by: Jeffrey Richter <jeffreyr@microsoft.com>

azure/ConsiderationsForServiceDesign.md

MushMal

Minor comments

azure/ConsiderationsForServiceDesign.md

MushMal · 2024-04-24T00:57:14Z

azure/Guidelines.md


 <a href="#lro-put-valid-inputs-synchronously" name="lro-put-valid-inputs-synchronously">:white_check_mark:</a> **DO** perform as much validation as practical when initiating the operation to alert clients of errors early.

 <a href="#lro-put-returns-200-or-201" name="lro-put-returns-200-or-201">:white_check_mark:</a> **DO** return a `201-Created` status code for create or `200-OK` for replace from the initial request with a representation of the resource if the resource was created successfully.

 <a href="#lro-put-returns-operation-id-header" name="lro-put-returns-operation-id-header">:white_check_mark:</a> **DO** include an `Operation-Id` header in the response with the ID of the status monitor for the operation.

-<a href="#lro-put-response-headers" name="lro-put-response-headers">:white_check_mark:</a> **DO** include response headers with any additional values needed for a GET request to the status monitor (e.g. location).
-
 <a href="#lro-put-returns-operation-location" name="lro-put-returns-operation-location">:ballot_box_with_check:</a> **YOU SHOULD** include an `Operation-Location` header in the response with the absolute URL of the status monitor for the operation.



Thinking about other edge cases such as expired and auto-deleted status monitor case.

azure/ConsiderationsForServiceDesign.md

weidongxu-microsoft · 2024-04-24T10:49:52Z

azure/ConsiderationsForServiceDesign.md

+that contains a representation of the status monitor _and_ any information from the request used to initiate the operation.
+
+The service is responsible for purging the status monitor after some period of time.
+It should auto-purge this resource after completion (at least 24 hours).


nit, this sentence may need rephase.

Maybe "This status monitor resource should be retained for at least 24 hours after completion. The service should auto-purge it afterward."

azure/Guidelines.md

weidongxu-microsoft · 2024-04-24T11:14:35Z

azure/Guidelines.md


-<a href="#lro-operation-id-request-header" name="lro-operation-id-request-header">:white_check_mark:</a> **DO** allow the client to pass an `Operation-Id` header with an ID for the operation's status monitor.
+<a href="#lro-put-response-headers" name="lro-put-response-headers">:white_check_mark:</a> **DO** include response headers with any additional values needed for a [GET polling request](#lro-poll) to the status monitor (e.g. location).


I think this line is moved here without change.

However, it is hard for me to understand the example:

include response headers with any additional values needed for a GET polling request to the status monitor (e.g. location).

I take the "e.g. location" mean the location header. But a location header seems not mean much to the GET polling request. (the polling URL should be from operation-location header that covered in the sentence above.

Do we have a better example? retry-after header is also mentioned in later point. operation-id header?
If no good example, maybe skip the example part of the sentence.

I agree that the example is poorly explained. The idea is that values passed in headers can be used as parameters -- specifically path parameters -- to the status monitor endpoint. There will always be an operation-id -- the final path segment -- but there may be other path parameters and headers can be used to specify what values should be used for these.

I don't know that we should go into all that detail in the guidelines doc, but perhaps we should include this in the ConsiderationsForServiceDesign.

azure/Guidelines.md

azure/ConsiderationsForServiceDesign.md

azure/Guidelines.md

@weidongxu-microsoft

Thanks @weidongxu-microsoft for all the great suggestions! Co-authored-by: Weidong Xu <weidxu@microsoft.com>

JeffreyRichter requested review from tg-msft, johanste, mikekistler and a team January 17, 2024 18:54

mikekistler reviewed Jan 19, 2024

View reviewed changes

heaths requested changes Jan 19, 2024

View reviewed changes

weidongxu-microsoft reviewed Jan 25, 2024

View reviewed changes

mikekistler mentioned this pull request Jan 28, 2024

Add guidelines on returning string offsets & lengths #521

Merged

JeffreyRichter and others added 2 commits March 17, 2024 10:09

Added LRO guidelines

97b50d1

Restore all prior named guidelines with some edits where needed

372bcd6

mikekistler force-pushed the LRO branch from f11b219 to 372bcd6 Compare March 17, 2024 19:18

heaths requested changes Mar 18, 2024

View reviewed changes

mikekistler changed the title ~~Added LRO & Substring guidelines~~ Updated LRO guidelines Apr 3, 2024

Address more PR review feedback

fc4193d

heaths approved these changes Apr 3, 2024

View reviewed changes

azure/Guidelines.md Outdated Show resolved Hide resolved

More fixes

0b5e4ea

heaths approved these changes Apr 5, 2024

View reviewed changes

azure/Guidelines.md Outdated Show resolved Hide resolved

johanste reviewed Apr 5, 2024

View reviewed changes

azure/ConsiderationsForServiceDesign.md Outdated Show resolved Hide resolved

johanste reviewed Apr 5, 2024

View reviewed changes

azure/ConsiderationsForServiceDesign.md Outdated Show resolved Hide resolved

johanste reviewed Apr 5, 2024

View reviewed changes

azure/ConsiderationsForServiceDesign.md Show resolved Hide resolved

More updates for PR comments

f247aad

weidongxu-microsoft reviewed Apr 7, 2024

View reviewed changes

azure/Guidelines.md Show resolved Hide resolved

weidongxu-microsoft reviewed Apr 8, 2024

View reviewed changes

azure/Guidelines.md Show resolved Hide resolved

weidongxu-microsoft approved these changes Apr 10, 2024

View reviewed changes

JeffreyRichter commented Apr 11, 2024

View reviewed changes

mikekistler and others added 2 commits April 14, 2024 14:07

Apply suggestions from PR review

8f9e591

Co-authored-by: Jeffrey Richter <jeffreyr@microsoft.com>

A few more small fixes

ee199b3

johanste reviewed Apr 15, 2024

View reviewed changes

azure/ConsiderationsForServiceDesign.md Show resolved Hide resolved

Another round of 'final' fixes

188ec81

MushMal approved these changes Apr 24, 2024

View reviewed changes

weidongxu-microsoft reviewed Apr 24, 2024

View reviewed changes

mikekistler mentioned this pull request Apr 30, 2024

Revisit response time requirements for non-LRO operations #542

Open

mikekistler reviewed Apr 30, 2024

View reviewed changes

azure/ConsiderationsForServiceDesign.md Outdated Show resolved Hide resolved

mikekistler reviewed Apr 30, 2024

View reviewed changes

azure/Guidelines.md Outdated Show resolved Hide resolved

Apply suggestions from PR review

9fb3343

Thanks @weidongxu-microsoft for all the great suggestions! Co-authored-by: Weidong Xu <weidxu@microsoft.com>

weidongxu-microsoft approved these changes May 6, 2024

View reviewed changes

One more fix

0217356

mikekistler merged commit c7d483c into microsoft:vNext May 6, 2024
1 check passed

mikeharder mentioned this pull request Jul 8, 2024

New API preview version for ACS Email Inline Attachment feature Azure/azure-rest-api-specs#29699

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated LRO guidelines #517

Updated LRO guidelines #517

JeffreyRichter commented Jan 17, 2024

mikekistler left a comment

mikekistler Jan 19, 2024

mikekistler Jan 19, 2024

mikekistler Jan 19, 2024

mikekistler Jan 19, 2024

JeffreyRichter commented Jan 19, 2024

heaths left a comment

heaths Jan 19, 2024

heaths Jan 19, 2024

weidongxu-microsoft Jan 25, 2024

heaths Jan 19, 2024

heaths Jan 19, 2024

heaths Jan 19, 2024

heaths Jan 19, 2024

heaths Jan 19, 2024

heaths Jan 19, 2024

weidongxu-microsoft Jan 25, 2024

weidongxu-microsoft Jan 25, 2024

mikekistler commented Apr 9, 2024

weidongxu-microsoft commented Apr 9, 2024 •

edited

Loading

MushMal left a comment

MushMal Apr 24, 2024

weidongxu-microsoft Apr 24, 2024

weidongxu-microsoft Apr 24, 2024 •

edited

Loading

mikekistler Apr 30, 2024

	## Patterns to Initiate a Long-Running Operation
	#### Patterns to Initiate a Long-Running Operation

	## The Status Monitor Resource
	#### The Status Monitor Resource


		<a href="#lro-status-monitor-includes-all-fields" name="lro-status-monitor-includes-all-fields">:white_check_mark:</a> DO include the `id` of the operation and any other values needed for the client to form a GET request to the status monitor (e.g. a `location` path parameter).
		## Pattern to Poll a Long-Running Operation's Status

	## Pattern to Poll a Long-Running Operation's Status
	#### Pattern to Poll a Long-Running Operation's Status


		<a href="#lro-status-monitor-retention" name="lro-status-monitor-retention">:white_check_mark:</a> DO retain the status monitor resource for some publicly documented period of time (at least 24 hours) after the operation completes.

		## Pattern to List Status Monitors (optional)

	## Pattern to List Status Monitors (optional)
	#### Pattern to List Status Monitors (optional)


		<a href="#lro-valid-inputs-synchronously" name="lro-valid-inputs-synchronously">:white_check_mark:</a> DO perform as much validation as practical when initiating the operation to alert clients of errors early.
		- The resource schema must contain an `initializationStatus` property indicating whether the initialization is `Running`, `Failed`, `Succeeded`, etc.

		@@ -1098,8 +1180,65 @@ While it may be tempting to use a revision/version number for the resource as th

		<a href="#condreq-etag-depends-on-encoding" name="condreq-etag-depends-on-encoding">:white_check_mark:</a> DO, when supporting multiple representations (e.g. Content-Encodings) for the same resource, generate different ETag values for the different representations.

		<a href="#substrings" name="substrings"></a>


		For example, if a service response needed to identify offset & length values for "name" and "email" substrings, the JSON response would look like this:

		```


		For a PUT (create or replace) with additional long-running processing:
		- Do not return a response body.


		<a href="#lro-operation-id-request-header" name="lro-operation-id-request-header">:white_check_mark:</a> DO allow the client to pass an `Operation-Id` header with an ID for the operation's status monitor.
		<a href="#lro-put-response-headers" name="lro-put-response-headers">:white_check_mark:</a> DO include response headers with any additional values needed for a [GET polling request](#lro-poll) to the status monitor (e.g. location).

Updated LRO guidelines #517

Updated LRO guidelines #517

Conversation

JeffreyRichter commented Jan 17, 2024

mikekistler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffreyRichter commented Jan 19, 2024

heaths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikekistler commented Apr 9, 2024

weidongxu-microsoft commented Apr 9, 2024 • edited Loading

MushMal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weidongxu-microsoft Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weidongxu-microsoft commented Apr 9, 2024 •

edited

Loading

weidongxu-microsoft Apr 24, 2024 •

edited

Loading